Skip to content

Conversation

@phlogistonjohn
Copy link
Collaborator

@phlogistonjohn phlogistonjohn commented Dec 14, 2025

Depends on: #229

Update the hack/build-image script to create new indexes/manifests that can represent >1 image with different architecture. In order to keep things simple and the changes to the script and CI yaml file smaller, I've extended the "FQIN" tagging to include a new 'any' arch value that is only used for indexes. Thus an index that covers both amd64 and arm64 builds might look like:
quay.io/samba.org/samba-server:default-fedora-any.
These indexes are constructed with a new --index option. The code for the --push code has been updated to support indexes with the new "QMatcher" flag values of index or index-multiarch the former pushes all indexes possible and the latter only pushes indexes if it contains images of >1 architecture.

The updates to the script are followed by updates to the CI YAML file to enable building and pushing the manifests. This also includes a switch to always use podman for building (for consistency) and to use native arm64 builders rather than emulation.


I'm using the wip branch at https://github.com/phlogistonjohn/samba-container to test these patches. You can see results of pushing to wip and the CI runs at https://github.com/phlogistonjohn/samba-container/actions
and the results get pushed to https://quay.io/repository/phlogistonjohn/samba-server?tab=tags and related repos under my user at quay.io.

@dpulls
Copy link

dpulls bot commented Dec 15, 2025

🎉 All dependencies have been resolved !

@phlogistonjohn phlogistonjohn marked this pull request as ready for review December 15, 2025 21:42

env:
CONTAINER_CMD: docker
CONTAINER_CMD: podman
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree here, and we should probably align to podman in all of samba-in-kubernetes sub-projects. However, it would be nice to add to the checks stage something like run: command -v ${{ env.CONTAINER_CMD }}

synarete
synarete previously approved these changes Dec 16, 2025
Copy link
Collaborator

@synarete synarete left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, looks good. My only concern is how we (developers) can test those images (outside of CI), but this discussion is out-of-scope for this PR.

@phlogistonjohn
Copy link
Collaborator Author

Overall, looks good. My only concern is how we (developers) can test those images (outside of CI), but this discussion is out-of-scope for this PR.

Indeed. I am hoping that once the new Ceph lab is sorted out there will be arm64 boxes to hop on and test these builds. Esp. since my plan is to eventually switch the cephadm code to make use of the -any indexes :-)

Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On top of the suggested changes I recommend enabling some CentOS based arm64 jobs -- anoopcs9@0cdfe83.

@phlogistonjohn
Copy link
Collaborator Author

On top of the suggested changes I recommend enabling some CentOS based arm64 jobs -- anoopcs9@0cdfe83.

Discussed in a call. I do believe that the PR has satisfied what we discussed and the follow up PR #234 ought to complete the needed changes suggested in the above patch.

Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On top of the suggested changes I recommend enabling some CentOS based arm64 jobs -- anoopcs9@0cdfe83.

Discussed in a call. I do believe that the PR has satisfied what we discussed and the follow up PR #234 ought to complete the needed changes suggested in the above patch.

Fine, still I think the other inline suggestions needs to be fixed (see below)

@phlogistonjohn
Copy link
Collaborator Author

FWIW I plan on adding a new option that allows secifiying an input file instead of a bunch of -i options in the future. That will rid the script of the duplication and allow us to use one "list".

@phlogistonjohn
Copy link
Collaborator Author

I don't know if it's github being weird or I'm just a blockhead today but I tried to make sense of your suggestions but couldn't. I used unix tools to ensure there were no more duplicates in the images and then I went over the samba-client "subsection" and tried fixing it up (and then copying it to the repeated sections). I'm testing it but I can't be sure it exactly covers what you are asking for.

Comment on lines 124 to 126
exclude:
- os: centos
arch: arm64
runs-on: ubuntu-latest
arch: {name: arm64, host: ubuntu-24.04-arm}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

samba-client:default-centos-arm64 is mentioned down the line. Did you forget to remove the exclude altogether here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. PTAL

Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks.

@anoopcs9
Copy link
Collaborator

@Mergifyio rebase

The lines were causing flake8 to complain so if we want to
use the tool to validate things we should make it happy.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Avoid syntax errors and style/formatting drift by adding a very
basic rule for flake8 and pyflakes to run in our CI.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
In preparation for being able to create indexes (aka manifests)
which can combine one or more "true" images the main function
being purely per-target will become limiting. Create a new helper
class structure that handles per target and then global actions
with a simple shim class for adapting the legacy functions without
needing to change them.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Add a new --index action that creates indexes/manifests based on
the inputted targets. For now the indexes abstract away the
architecture.
To preserve the FQIN pattern our indexes and indexes alone will
use 'any' in the 3rd position of the FQIN tag.
This will allow consumers to eventually run something like
`podman pull quay.io/samba.org/samba-server:default-fedora-any`
and it will automatically select the correct image for the
architecture if said arch image exists. This does NOT change
the existing per-arch FQIN for "real" images.

My experiments with manifest commands with podman have shown
some quirky behavior so you will see a more belt-and-suspenders
approach to some of these commands. I have only tested it on
podman because I don't care about docker.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Swap the push function to a new Pusher class to make use of the
new "main function" handling in preparation for extending push
to also push indexes/manifests.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Enhance the QMatcher class to help be a more generic "selector"
for what to push. This is in preparation for pushing indexes.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Add support to push indexes/manifests along with "real" images.
This can be invoked by specifying either "index" or "index-multiarch"
to the --push-kinds/--push-what option. For example:
`--push-kinds=mixed,index-multiarch`.
Both options enable pushing indexes (aka manifests) but
"index-multiarch" will only push those that refer to 2 or more
"real" images - implying images that actually have support
for multiple architectures.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Use podman in our CI because it matches what we (I?) use
to test the scripts and what I use to release images.
I'm not against having things work with docker but
I don't like having to constantly second guess what I
am doing when working on these features.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
GitHub has supported native arm64 for almost a year, but that
had somehow flown under my radar until recently. Tweak the
matrix to choose those builders when building for arm64 instead
of relying on emulation.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Indexes, aka manifests, make it easier for clients to specify a
tag name that will automatically select between architectures for
you when choosing an image from a registry.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
We test the opensuse images but we hadn't been pushing them before
but somehow some of the arm64 (and only arm64!) images got added
to the list of images to publish to our quay.io registry.
Stop that.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Try to make the list of images that we publish a bit more
obvious by sorting the lists.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
It was excluded without needing to be.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Signed-off-by: John Mulligan <jmulligan@redhat.com>
@mergify
Copy link

mergify bot commented Dec 21, 2025

rebase

✅ Branch has been successfully rebased

@phlogistonjohn
Copy link
Collaborator Author

@synarete ptal, thanks

@phlogistonjohn
Copy link
Collaborator Author

Reminder: the jobs names change so we'll need to manually merge it. Once it's merged we (I) need to update the branch protection rules with the new names.

@anoopcs9
Copy link
Collaborator

Reminder: the jobs names change so we'll need to manually merge it. Once it's merged we (I) need to update the branch protection rules with the new names.

It won’t be just the branch‑protection rules; the Mergify configuration would also need an update (if so, it’s better to include those changes here). Otherwise, we could explicitly define job names as: name: ${{ matrix.package_source }}‑${{ matrix.os }}‑${{ matrix.arch.name }}.

@phlogistonjohn phlogistonjohn merged commit a570626 into samba-in-kubernetes:master Dec 23, 2025
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants